Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mellanox][pcied] Ignore bus on pcie.yaml for Mellanox switches #8063

Merged
merged 1 commit into from
Jul 26, 2021

Conversation

DavidZagury
Copy link
Contributor

@DavidZagury DavidZagury commented Jul 5, 2021

Why I did it

BIOS upgrade on rare cases cannot guarantee bus value remain the same on every BIOS release. Ignoring this field in order for pcied not to fail but still verify device id in a different way. The solution is future proof and will not require changes in code when new BIOS version is available

How I did it

Since bus is not a fixed value (it is determined by the bios version) we are ignoring this field, and instead checking if there is a device that match on all other fields that and in addition has a matching device id.

How to verify it

Verify no errors or failures in pcied on different BIOS version with the same code base.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

@DavidZagury DavidZagury requested a review from lguohan as a code owner July 5, 2021 16:41
@lgtm-com
Copy link

lgtm-com bot commented Jul 5, 2021

This pull request introduces 1 alert when merging 06ce2c37a8aea536329e32ca230b0d9271fa40db into fe6e4c3 - view on LGTM.com

new alerts:

  • 1 for Unused import

@BasimShalata
Copy link
Contributor

@sujinmkang can you please review.
its based on your changes and as we discussed in our previous meeting

@liat-grozovik liat-grozovik requested review from sujinmkang and keboliu and removed request for lguohan July 6, 2021 10:10
@DavidZagury DavidZagury force-pushed the master_pcie branch 2 times, most recently from f1e2a32 to 7cb1cc2 Compare July 6, 2021 13:50
keboliu
keboliu previously approved these changes Jul 9, 2021
for folder in device_folders:
# For each folder in the sysfs tree we check if it matches the normal PCIe device folder pattern,
# If match we add the device id from the device file and the bus from the folder name to the map
pattern_for_device_folder = re.search('....:(..):..\..', folder)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

....:(..):....

This regex is hard to read. Could you give some examples and explanations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example for a dir name is: 0000:ff:0b.1
The name of the folder is created from: {4 digit of domain - always 0}:{2 hex digit of bus}:{2 hex digit - dev}.{fn}

You can see this pattern of the folder name also here:
https://github.com/Azure/sonic-platform-common/blob/87c81de7193ee68568543d982fb8ba38f5e4ad07/sonic_platform_base/sonic_pcie/pcie_common.py#L106

I use this regex to look for the bus (in the above example 'ff').
The dots match any character, so I am checking if the folder that starts with 4 characters (domain), colon, 2 characters of bus (they are in parentheses to create the group I am looking for), a colon, 2 characters of dev, a dot, and the fn.

I tried to create the simplest regex, but I would be glad to hear if you have a different suggestion

if pattern_for_device_folder:
bus = pattern_for_device_folder.group(1)
with open(os.path.join('/sys/bus/pci/devices', folder, 'device'), 'r') as device_file:
device_id = device_file.read().replace('\n', '').replace('0x', '')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

device_id = device_file.read().replace('\n', '').replace('0x', '')

Could you give a sample file content? If future change happens, we know what is broken and what is expected output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example of the contact of a file:

0x6fe2

I am looking for the '6fe2' part only. I will add a comment regarding this, and maybe will make it simpler by changing the first replace to strip

qiluo-msft
qiluo-msft previously approved these changes Jul 19, 2021
@sujinmkang sujinmkang merged commit 67781ab into sonic-net:master Jul 26, 2021
qiluo-msft pushed a commit that referenced this pull request Jul 27, 2021
Why I did it
BIOS upgrade on rare cases cannot guarantee bus value remain the same on every BIOS release. Ignoring this field in order for pcied not to fail but still verify device id in a different way. The solution is future proof and will not require changes in code when new BIOS version is available

How I did it
Since bus is not a fixed value (it is determined by the bios version) we are ignoring this field, and instead checking if there is a device that match on all other fields that and in addition has a matching device id.

How to verify it
Verify no errors or failures in pcied on different BIOS version with the same code base.
judyjoseph pushed a commit that referenced this pull request Aug 4, 2021
Why I did it
BIOS upgrade on rare cases cannot guarantee bus value remain the same on every BIOS release. Ignoring this field in order for pcied not to fail but still verify device id in a different way. The solution is future proof and will not require changes in code when new BIOS version is available

How I did it
Since bus is not a fixed value (it is determined by the bios version) we are ignoring this field, and instead checking if there is a device that match on all other fields that and in addition has a matching device id.

How to verify it
Verify no errors or failures in pcied on different BIOS version with the same code base.
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
…c-net#8063)

Why I did it
BIOS upgrade on rare cases cannot guarantee bus value remain the same on every BIOS release. Ignoring this field in order for pcied not to fail but still verify device id in a different way. The solution is future proof and will not require changes in code when new BIOS version is available

How I did it
Since bus is not a fixed value (it is determined by the bios version) we are ignoring this field, and instead checking if there is a device that match on all other fields that and in addition has a matching device id.

How to verify it
Verify no errors or failures in pcied on different BIOS version with the same code base.
@DavidZagury DavidZagury deleted the master_pcie branch May 5, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants